TDL-17512: Add missing tap-tester tests by hpatel41 · Pull Request #134 · singer-io/tap-shopify

hpatel41 · 2022-01-31T12:59:45Z

Description of change

TDL-17512: Add missing tap-tester tests

added all fields test
missing assertions in discovery, start date, bookmark test cases.

QA steps

automated tests passing
manual qa steps passing (list below)

Risks

Rollback steps

revert this branch

savan-chovatiya · 2022-02-02T07:23:42Z

+        # removed "abandoned_checkouts", as per the Doc:
+        #   https://help.shopify.com/en/manual/orders/abandoned-checkouts?st_source=admin&st_campaign=abandoned_checkouts_footer&utm_source=admin&utm_campaign=abandoned_checkouts_footer#review-your-abandoned-checkouts
+        # abandoned checkouts are saved in the Shopify admin for three months.
+        # Every Monday, abandoned checkouts that are older than three months are removed from your admin.
+        return set(self.expected_metadata().keys()) - {"abandoned_checkouts"}


Add comments about the unavailability of the POST call with a reference link.

Added comment in the test cases.

savan-chovatiya · 2022-03-03T10:57:10Z

+                    if md_entry['breadcrumb'] != []:
+                        actual_fields.append(md_entry['breadcrumb'][1])
+                # Verify there are no duplicate/conflicting metadata entries.
+                self.assertEqual(len(actual_fields), len(set(actual_fields)), msg="There are duplicate entries in the fields of '{}' stream".format(stream))


Add a comment about this assertion at the function(test_run()) level comment above.

Added function comment.

Good addition

savan-chovatiya · 2022-03-03T10:58:43Z

-                target_mark = second_min_bookmarks.get(stream, {"mark": None})
-                target_value = next(iter(target_mark.values()))  # there should be only one
+                # Verify by primary key values, that all records of the 2nd sync are included in the 1st sync since 2nd sync has a later start date.
+                self.assertTrue(primary_keys_sync_2.issubset(primary_keys_sync_1))


Add a comment about this assertion at the function(test_run()) level comment above.

Added function comment.

savan-chovatiya · 2022-03-03T10:59:45Z

+                for start_date, target_mark in zip((first_sync_start_date, second_sync_start_date), (first_sync_target_mark, second_sync_target_mark)):
+                    target_value = next(iter(target_mark.values()))  # there should be only one


Add a comment here about the addition of for loop.

Added comment.

dbshah1212 · 2022-03-08T12:18:00Z

+        return return_value
+
+    @staticmethod
+    def get_credentials(original_credentials: bool = True):


It is already available in base.py, Any reason for redefining it?

Updated the test case to use the function from base test file.

kspeer825

This is good work. I left one suggestion for the start date test. Also the abandoned_checkouts does not need to be removed. We periodically refresh that data using tooling outside of tap-tester.

kspeer825 · 2022-04-08T12:26:52Z

-        return set(self.expected_metadata().keys())
+        # removed "abandoned_checkouts", as per the Doc:
+        #   https://help.shopify.com/en/manual/orders/abandoned-checkouts?st_source=admin&st_campaign=abandoned_checkouts_footer&utm_source=admin&utm_campaign=abandoned_checkouts_footer#review-your-abandoned-checkouts
+        # abandoned checkouts are saved in the Shopify admin for three months.
+        # Every Monday, abandoned checkouts that are older than three months are removed from your admin.
+        # Also no POST call is available for this endpoint: https://shopify.dev/api/admin-rest/2022-01/resources/abandoned-checkouts
+        return set(self.expected_metadata().keys()) - {"abandoned_checkouts"}


Please put this stream back, we have internal tooling for generating these records outside of the base test suite.

Included abandoned_checkouts stream

kspeer825 · 2022-04-08T12:27:14Z

        self.start_date = self.get_properties().get("start_date")
        self.store_1_streams = {'custom_collections', 'orders', 'products', 'customers', 'locations', 'inventory_levels', 'inventory_items', 'events'}
-        self.store_2_streams = {'abandoned_checkouts', 'collects', 'metafields', 'transactions', 'order_refunds', 'products', 'locations', 'inventory_levels', 'inventory_items', 'events'}
+        # removed 'abandoned_checkouts' from store 2 streams, as per the Doc:


Please put back, see previous comment

Included abandoned_checkouts stream

kspeer825 · 2022-04-08T12:27:58Z

                    msg="The fields sent to the target are not the automatic fields"
                )
+
+                # Verify that all replicated records have unique primary key values.


Good addition

kspeer825 · 2022-04-08T12:29:19Z

+                    if md_entry['breadcrumb'] != []:
+                        actual_fields.append(md_entry['breadcrumb'][1])
+                # Verify there are no duplicate/conflicting metadata entries.
+                self.assertEqual(len(actual_fields), len(set(actual_fields)), msg="There are duplicate entries in the fields of '{}' stream".format(stream))


Good addition

kspeer825 · 2022-04-08T12:52:03Z

+                first_sync_start_date = self.get_properties()["start_date"]
+                second_sync_start_date = self.start_date
+
+                # loop over minimum bookmark and the start date/state file date for each syncs to verify


The original implementation here was very difficult to follow. These changes are helpful, but can this be updated to conform to current best practices? See https://github.com/singer-io/tap-helpscout/blob/ad2b6192a5f5fad8c5bd0573cb38bf38bd5962dd/tests/test_helpscout_start_date.py#L109-L122

The first 2 assertions are already included in our test case at line no. 141 and 147. Added assertion of replication key-values synced is greater than or equal to the start date.

kspeer825 · 2022-04-11T12:52:29Z

+                    for record_replication_key in record_replication_keys:
+                        self.assertGreaterEqual(record_replication_key, start_date)


I think the try except and assertion above are unnecessary. The minimum replication key value is covered by this assertion.

Removed unnecessary assertion.

harshpatel4_crest added 2 commits January 31, 2022 18:28

added missing test cases

b7b0d79

removed abandoned_checkouts from all test cases except discovery

7fd7010

hpatel41 changed the base branch from master to crest-work February 2, 2022 06:03

savan-chovatiya marked this pull request as ready for review February 2, 2022 07:23

savan-chovatiya requested review from dbshah1212, karanpanchal-crest and savan-chovatiya February 2, 2022 07:23

savan-chovatiya reviewed Feb 2, 2022

View reviewed changes

added comment for abandoned checkouts in test cases

3088df8

hpatel41 requested a review from savan-chovatiya February 2, 2022 09:02

savan-chovatiya suggested changes Mar 3, 2022

View reviewed changes

added code comments and function comments

3daafa7

hpatel41 requested a review from savan-chovatiya March 4, 2022 05:41

dbshah1212 reviewed Mar 8, 2022

View reviewed changes

Comment thread tests/test_all_fields.py

dbshah1212 reviewed Mar 8, 2022

View reviewed changes

Comment thread tests/test_start_date.py Outdated

updated all fields test

273ce0e

hpatel41 requested a review from dbshah1212 March 9, 2022 08:42

savan-chovatiya approved these changes Mar 10, 2022

View reviewed changes

dbshah1212 approved these changes Mar 24, 2022

View reviewed changes

dbshah1212 requested review from KrisPersonal, RushiT0122 and kspeer825 April 7, 2022 05:37

kspeer825 suggested changes Apr 8, 2022

View reviewed changes

include abandoned_checkouts stream and updated start date test case

7bfc94e

hpatel41 requested a review from kspeer825 April 11, 2022 10:23

kspeer825 suggested changes Apr 11, 2022

View reviewed changes

removed unnecessary assertion in start date test

012242e

hpatel41 requested a review from kspeer825 April 11, 2022 13:46

hpatel41 changed the base branch from crest-work to api-sdk-version-change-and-fields-addition April 12, 2022 07:02

hpatel41 changed the base branch from api-sdk-version-change-and-fields-addition to crest-work April 12, 2022 07:02

kspeer825 approved these changes Apr 13, 2022

View reviewed changes

namrata270998 mentioned this pull request Apr 22, 2022

Tdl 15459 update bookmarking for transactions #141

Open

RushiT0122 approved these changes May 6, 2022

View reviewed changes

		for start_date, target_mark in zip((first_sync_start_date, second_sync_start_date), (first_sync_target_mark, second_sync_target_mark)):
		target_value = next(iter(target_mark.values())) # there should be only one

		for record_replication_key in record_replication_keys:
		self.assertGreaterEqual(record_replication_key, start_date)

Conversation

hpatel41 commented Jan 31, 2022

Description of change

QA steps

Risks

Rollback steps

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hpatel41 Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kspeer825 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hpatel41 Apr 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hpatel41 Mar 9, 2022 •

edited

Loading

hpatel41 Apr 11, 2022 •

edited

Loading